-
Notifications
You must be signed in to change notification settings - Fork 943
Added some unit tests for memori._cli.py #249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for putting this together. I appreciate you taking the time to add coverage around the CLI helpers. One concern on my end is that these tests are primarily asserting exact print() output. In practice that tends to be a bit brittle (small copy/format tweaks can break CI) and it doesn’t really exercise the failure modes we'd be concerned with in the cli; argument parsing, command wiring, exit codes, and error paths. Would you be open to adjusting the tests toward higher signal behavior? For example, invoking the CLI/entrypoint in a more end-to-end way (args → command execution) and asserting on exit codes and a couple key outcomes, while keeping output checks looser (e.g., version is present, important keywords show up) rather than matching exact strings. |
|
Hey @devwdave, Thanks for the thoughtful feedback. You’re absolutely right. I agree that asserting exact print() output is brittle and low-signal. I’m happy to move forward with your suggestion and refactor the tests toward higher-level CLI behavior by invoking the entrypoint with arguments, asserting on exit codes, command wiring, and error paths, while keeping output checks loose (presence of key markers rather than exact matches). I’ll push an updated commit reflecting this shortly. |
Add comprehensive end-to-end CLI tests - Added 16 comprehensive CLI tests covering help variations, command validation, and error handling - Implemented tests for CockroachDB cluster management commands (start, claim, delete) - Added ASCII logo display verification test - Used parametrized tests for multiple help invocation methods (no args, --help, -h, help) - Refactored tests to use behavior-based assertions instead of exact string matching - Improved test assertions to validate both exit codes and error messages - Updated CHANGELOG.md with test improvements Addresses reviewer feedback on PR MemoriLabs#249 to make tests less brittle and more behavior-focused.
- Added 16 comprehensive CLI tests covering help variations, command validation, and error handling - Implemented tests for CockroachDB cluster management commands (start, claim, delete) - Added ASCII logo display verification test - Used parametrized tests for multiple help invocation methods (no args, --help, -h, help) - Refactored tests to use behavior-based assertions instead of exact string matching - Improved test assertions to validate both exit codes and error messages - Updated CHANGELOG.md with test improvements Addresses reviewer feedback on PR MemoriLabs#249 to make tests less brittle and more behavior-focused.
23908b4 to
19dbc1b
Compare
|
Hi @devwdave, Thanks for the detailed feedback. I’ve refactored the tests based on your suggestions. Below is a quick summary of what I changed. What ChangedEnd-to-End Testing
More Behavior-Focused Assertions
Expanded CoverageAdded 16 tests covering:
Test Quality Improvements
|
Hey everyone! This is my very first contribution to Memori. I've added some unit tests for the _cli.py module to make sure our command line output, like the banner and notices, always looks and behaves exactly as expected.